Respect CLJ_JVM_OPTS env var options when downloading clojure-tools #73

ikappaki · 2022-10-24T21:55:26Z

Hi,

can you please consider patch to try and download clojure tools archive via a java subprocess that respects the CLJ_JVM_OPTS environment variable. This addresses #66.

I have disabled the babashka tests because it can't recognise the new imported java.net.URLConnection symbol, would it be possible to add it to the babashka arsenal please if review is successful please?

5:   (:import [java.lang ProcessBuilder$Redirect]
     ^--- Unable to resolve classname: java.net.URLConnection

Changes are:

The script will now attempt to download the clojure tools using the java executable first, fallback to direct download, and if that fails, display a message to the user where to copy the zip archive to. The java download methods requires Java11+/JEP-330.
If the archive was there though (say the user copied it over), the tool will not try to download it again, but unzip it.
An issue was identified in unzip with Filesystems/newFilesystem that was never closed (it is now in a with-open form).
An inconsistency was identified that tools-dir was passed to clojure-tools-jar-download as a destination directory instead of libexec-dir.
The HttpURLConnection is only set when downloading the clojure tools archive, if the src URL is http. For testing we use file:// where possible. This introduced the java.net.URLConnection symbol, but unfortunately it is not available in babashka and thus the bb tests have been temporarily disabled until this is resolved.
Tests have been introduced to test all clojure tools download methodologies. A new dummy clojure tools archive is created on the fly when needed, and some java downloader tests only run when java > 11.
Updated README.md mentioned the new env variables and other minor corrections.

Thanks,

src/borkdude/deps.clj

test/borkdude/deps_test.clj

borkdude · 2022-10-25T09:15:54Z

@ikappaki Beautiful PR, just a few minor comments.

For testing bb, I added java.net.URLConnection to bb now and you can get a dev build from:

https://github.com/babashka/babashka-dev-builds/releases/tag/v1.0.165-SNAPSHOT

You can install this with the installer script as well by providing a version with:

curl -sLO https://raw.githubusercontent.com/babashka/babashka/master/install
chmod +x ./install
./install --version 1.0.165-SNAPSHOT

Please update CI and turn bb tests back on.

1. Convert inline java program as a single string. 2. Simplify formatting of download msgs. 3. Only display a single msg while downloading, and only print out the other message when new DEPS_CLJ_DBG env var is set. 4. Correct typos. 5. Update CI to use bb 1.0.165-SNAPSHOT for this review purposes and restore bb tests in CI.

borkdude · 2022-10-25T19:45:19Z

src/borkdude/deps.clj

-                                                                ["-Xms256m"])))
+           (let [java-clj-jvm-opts (when clj-jvm-opts (vec (concat clj-jvm-opts
+                                                                   proxy-settings
+                                                                   ["-Xms256m"])))]


What is the -Xms256m argument for?

I picked it up from the clj-main-cmd a little bit further down the code, I assumed it was for some unknown to me use, shall I remove it?

ikappaki · 2022-10-25T19:56:34Z

@ikappaki Beautiful PR, just a few minor comments.

For testing bb, I added java.net.URLConnection to bb now and you can get a dev build from:

https://github.com/babashka/babashka-dev-builds/releases/tag/v1.0.165-SNAPSHOT

You can install this with the installer script as well by providing a version with:
curl -sLO https://raw.githubusercontent.com/babashka/babashka/master/install
chmod +x ./install
./install --version 1.0.165-SNAPSHOT
Please update CI and turn bb tests back on.

Hi @borkdude,

I've addressed most of the review comments and updated the CI configs to use the babashka snapshot (markes as REVIEW) so as to run the babashka-test task. I'm ready to revert back to the normal babashka CI installation when you are ready to publish a new version.

In addition I've added some comments to the -main docstring about the use of the new variables, if you don't mind me intruding.

The only thing I haven't address is determining the java version, can you have a look at my reply and available options please?

Thanks,

borkdude · 2022-10-25T20:31:48Z

I think this looks good. I looked into the Xms256 thing and really can't find where this came from. This is the initial commit of the deps.clj script and it was already there. I think I'll just remove it since it isn't there in the clojure script either.

borkdude · 2022-10-25T20:32:21Z

Much thanks!

Replaces snapshot introduced in borkdude#73

Replaces snapshot introduced in #73 Co-authored-by: ikappaki <[email protected]>

Respect CLJ_JVM_OPTS env var options when downloading clojure-tools

f36e4a2